Refactor: instruments have sensors by j-atkins · Pull Request #328 · Parcels-code/virtualship

j-atkins · 2026-04-10T11:44:26Z

Overview

This PR centralises/abstracts instrument sensor/variable sampling logic so that each instrument declares which sensors it carries (e.g. TEMPERATURE, SALINITY, VELOCITY). Users can configure which sensors are active for each instrument in the expedition YAML.

This helps pave the way for easy addition of BGC sensors to the Argo float in a future PR (#234), and consolidation of CTD + CTD_BGC into a single instrument (#260) with a combined sensor allowlist. Also makes it straightforward to add new sensors to any instrument in the future (e.g., #312, #313), and streamlines them process for developers to add new instruments (i.e. #237)

Major changes

sensors.py in instruments/ defines the SensorType class and per-instrument allowlists (so that there is control over which sensors each instrument supports and users cannot configure unsupported sensors).
New SensorConfig pydantic model and sensors field in every instrument config in expedition.py.
New SensorRegistry in utils.py that maps each SensorType to its FieldSet key, Copernicus variable name, category (phys/bgc), and Parcels particle variable name(s).
- Per-instrument parcticle classes are now built dynamically at runtime based on which sensors are active, but the fixed/mechanical variables are still hard-coded in the instrument files, e.g. cycle_phase for Argo Floats.

API change

As mentioned above, the instruments config section of the expedition YAML now has a sensors list field, where users specify which sensors are active. For example, below the CTD is configured to sample TEMPERATURE and SALINITY:

ctd_config:
  stationkeeping_time_minutes: 50
  min_depth_meter: -11
  max_depth_meter: -2000
  sensors:
    - TEMPERATURE
    - SALINITY

If the sensors list is omitted, it default to all valid sensors for that instrument.
By using allowlists for each instrument, it will not allow non-sensical sensor combinations (e.g. BGC sensors on an ADCP).
Will also not allow an empty sensor list, at least one sensor must be active.

Additional change

Argo Float sampling kernels have been separated from the vertical-movement kernel, making it easier to add BGC sensors in a future PR.

Follow-up PRs

The plan CLI tool will need updating to account for sensor configuration options. Currently not broken but doesn't give option to configure sensors. (New issue to be opened)
Docs update to clearly communicate which sensors each instrument accepts, and how to configure them in either the plan tool or the expedition YAML. (New issue to be opened)
Merge CTD + CTD_BGC into a single instrument. (Unify CTD and CTD_BGC to one instrument #260)
Add BGC sampling to Argo Floats (New ARGO_BGC instrument #234)

Tests

Update existing tests and add new tests to cover new sensor logic

…pe enum

…_bgc

…ensors for instruments

…ated validations for each instrument

…igher level for scalability

… kernels from the argo vertical movement kernel to enable easier scalability

…operty shorthands

…pe enum

…_bgc

…ensors for instruments

…ated validations for each instrument

…igher level for scalability

…operty shorthands

…p into refactor-sensors

…figurations

for more information, see https://pre-commit.ci

…p into refactor-sensors

erikvansebille

Looks good! A few comments below

erikvansebille · 2026-04-13T06:18:24Z

There seems to still be a lot of duplication between CTD and CTD_BGC. Was the intention for this PR not to remove that duplication/ Or will that come in a next PR?

Yes, indeed it will come in the next PR... the CTD_BGC will soon be removed but I decided not to do it in this PR so as to keep it focused

erikvansebille · 2026-04-13T06:20:38Z

-        Variable("lifetime", dtype=np.float32),
-    ]
-)
+_DRIFTER_FIXED_VARIABLES = [


Not sure whether FIXED is the right term here (and in other instruments). Suggests they are fixed to the instrument. Would NONSENSOR be a better term? Or do you have another term?

erikvansebille · 2026-04-13T06:22:24Z

+    TEMPERATURE = "TEMPERATURE"
+    SALINITY = "SALINITY"
+    VELOCITY = "VELOCITY"
+    OXYGEN = "OXYGEN"
+    CHLOROPHYLL = "CHLOROPHYLL"
+    NITRATE = "NITRATE"
+    PHOSPHATE = "PHOSPHATE"
+    PH = "PH"
+    PHYTOPLANKTON = "PHYTOPLANKTON"
+    PRIMARY_PRODUCTION = "PRIMARY_PRODUCTION"


Why is this list needed? Can we do without? Seems like an extra place to change when adding a sensor..

I prefer to keep it just because it's a lightweight way of adding a bit of consistency/convention across the codebase and helps with some things like quickly catching typos in the YAML config

erikvansebille · 2026-04-13T06:23:51Z

+_ST_SENSOR_KERNELS: dict[SensorType, callable] = {
+    SensorType.TEMPERATURE: _sample_temperature,
+    SensorType.SALINITY: _sample_salinity,
+}


Why explicitly mention them here, if they are also part of sensors.py? Seems a duplication?

erikvansebille · 2026-04-13T06:25:04Z

 import yaml

 from virtualship.errors import InstrumentsConfigError, ScheduleError
+from virtualship.instruments.sensors import (


Why not import all? So that if a new sensor is added, it can automatically also be imported here?

To be clear, are you suggesting from virtualship.instruments import sensors? I think that would be a good idea :)

erikvansebille · 2026-04-13T06:28:45Z

+        """
+        FieldSet-key → Copernicus-variable mapping for enabled sensors.
+
+        VELOCITY is a special case: one sensor provides two FieldSet variables (U and V).


But in Parcels, it should sample fieldset.UV, which is one Field?

Oops! Overcomplicated things here unnecessarily

erikvansebille · 2026-04-13T06:29:53Z

+        ],
+        description=(
+            "Sensors fitted to the BGC CTD. "
+            "Supported: CHLOROPHYLL, NITRATE, OXYGEN, PH, PHOSPHATE, PHYTOPLANKTON, PRIMARY_PRODUCTION. "


Make this list the same order as the list above? Otherwise, more difficult to see whether the list is complete

erikvansebille · 2026-04-13T06:31:10Z

+    @pydantic.field_validator("sensors", mode="after")
+    @classmethod
+    def _check_sensor_compatibility(cls, value) -> list[SensorConfig]:
+        return _check_sensor_compatibility(value, XBT_SUPPORTED_SENSORS, "XBT")
+
+    @pydantic.field_serializer("sensors")
+    def _serialize_sensors(self, value: list[SensorConfig], _info):
+        return _serialize_sensor_list(value)
+
+    def active_variables(self) -> dict[str, str]:
+        """FieldSet-key → Copernicus-variable mapping for enabled sensors."""
+        return build_variables_from_sensors(self.sensors)
+


Why are these functions required for every instrument? Can't they be reused?

erikvansebille · 2026-04-13T06:36:21Z

+            particle_vars=[
+                "U",
+                "V",
+            ],  # two particle variables associated with one sensor


Why not UV?

erikvansebille · 2026-04-13T06:37:31Z

Great to add so much more unit testing!

VeckoTheGecko

Good work so far! Really like the direction that this is going. I have a bunch of comments, some quick fixes and others more cenceptual that I think would be good to go over together.

VeckoTheGecko · 2026-04-17T09:42:12Z

+class _SensorMeta:
+    fs_key: str  # map to Parcels fieldset variables
+    copernicus_var: str  # map to Copernicus Marine Service variable names


I think it would be good to rename this class to _Sensor, and to include the type in the class itself

Suggested change

class _SensorMeta:

fs_key: str # map to Parcels fieldset variables

copernicus_var: str # map to Copernicus Marine Service variable names

class _Sensor:

type_: SensorType

fs_key: str # map to Parcels fieldset variables

copernicus_var: str # map to Copernicus Marine Service variable names

Then the SENSOR_REGISTRY can become:

SENSOR_REGISTRY: dict[SensorType, _Sensor] = {s.type_: s for s in [ ... # list of sensors ]}

making it easy to look up sensors, but also making it so that if a sensor its chosen it's easy to see what type it is again.

VeckoTheGecko · 2026-04-17T09:42:20Z

+    PRIMARY_PRODUCTION = "PRIMARY_PRODUCTION"
+
+
+# per-instrument allowlists of supported sensors (source truth for validation for which sensors each instrument supports)


I find it a bit confusing the structure of the classes here - perhaps this is something worth drawing up so that we have some clarity?

We have an Instrument class

@register_instrument(InstrumentType.ADCP) class ADCPInstrument(Instrument): """ADCP instrument class.""" def __init__(self, expedition, from_data): """Initialize ADCPInstrument.""" variables = expedition.instruments_config.adcp_config.active_variables() limit_spec = { "spatial": True } # spatial limits; lat/lon constrained to waypoint locations + buffer super().__init__( expedition, variables, add_bathymetry=False, allow_time_extrapolation=True, verbose_progress=False, spacetime_buffer_size=None, limit_spec=limit_spec, from_data=from_data, )

But I feel that there's a lot of details in this class that aren't really related to what an instrument is, but is more glue code to get it working in VirtualShip

From my POV, its best to design things close to the physical domain. An Instrument instance has a set of sensors installed on the machine. These sensors need to match up with a list of allowed sensors (which are defined on the class, e.g., via a method .get_allowed_sensors() and a check in the __init__ that it adheres - this would remove all the _check_sensor_compatibility methods that are currently here). Then each sensor has its own details potentially related to that sensor.

Maybe it would be good for us to sit down and diagram out the abstractions here. Part of that might be brainstorming how the interface between the configs and model code looks like.

VeckoTheGecko · 2026-04-17T09:46:00Z

+
+
+@dataclass(frozen=True)
+class _SensorMeta:


Is it possible for us to also add the kernels to this Sensor class? I see a bunch of mappings across the codebase relating the sensors to the kernels (where a lot of them are the same, as Erik mentioned in #328 (comment) ).

I think it would be a big win if we can put the kernels in the sensor class! I don't know how easy it would be since the right kernel might depend on more than just the choice of sensor. If that's the case - what does choosing the right kernel depend on?

VeckoTheGecko · 2026-04-17T09:46:37Z

+# =====================================================
+# SECTION: sensor and variable metadata and registries
+# =====================================================


Should we move this to sensors.py? The class and the SENSOR_REGISTRY?

VeckoTheGecko · 2026-04-17T09:56:57Z

+    category: Literal[
+        "phys", "bgc"
+    ]  # physical vs. biogeochemical variable, used for product ID selection logic
+    particle_vars: list[str]  # particle variable name(s) produced by this sensor


I see that this is only used in

virtualship/src/virtualship/utils.py

Lines 746 to 751 in 89e184a

sensor_variables = [

Variable(var_name, dtype=np.float32, initial=np.nan)

for sc in sensors

if sc.enabled

for var_name in sc.meta.particle_vars

]

. Perhaps it would be clearer instead of these being strings for them to explicitly be lists of parcels.Variable objects?

Also, for my own understanding, how does this relate to (e.g.)

virtualship/src/virtualship/instruments/argo_float.py

Lines 38 to 49 in 89e184a

_ARGO_FIXED_VARIABLES = [

Variable("cycle_phase", dtype=np.int32, initial=0.0),

Variable("cycle_age", dtype=np.float32, initial=0.0),

Variable("drift_age", dtype=np.float32, initial=0.0),

Variable("min_depth", dtype=np.float32),

Variable("max_depth", dtype=np.float32),

Variable("drift_depth", dtype=np.float32),

Variable("vertical_speed", dtype=np.float32),

Variable("cycle_days", dtype=np.int32),

Variable("drift_days", dtype=np.int32),

Variable("grounded", dtype=np.int32, initial=0),

]

? Are the particle variables here "extra ones" needed for the sensor and to be captured as output, whereas the ones in _ARGO_FIXED_VARIABLES are for the base behaviour of the instrument?

VeckoTheGecko · 2026-04-17T10:01:04Z

+_ADCP_SENSOR_KERNELS: dict[SensorType, callable] = {
+    SensorType.VELOCITY: _sample_velocity,
+}


This type hint isn't correct

Suggested change

_ADCP_SENSOR_KERNELS: dict[SensorType, callable] = {

SensorType.VELOCITY: _sample_velocity,

}

from collections.abc.Callable # put at top of file

_ADCP_SENSOR_KERNELS: dict[SensorType, Callable] = {

SensorType.VELOCITY: _sample_velocity,

}

Great use of typing across the PR - I think having this typing is really useful so that it gets us to think more about the structure of the codebase. Currently I don't think typechecking is run in CI (or if it is, we currently have a lot of failures). Should we map out a path forward for enabling typechecking across the codebase as well? The types are only really useful if they're enforced (something I'm working on in Parcels as well).

VeckoTheGecko · 2026-04-17T10:02:54Z

@@ -50,6 +53,11 @@ def _sample_temperature(particle, fieldset, time):
    particle.temperature = fieldset.T[time, particle.depth, particle.lat, particle.lon]




Same here about the callable type annotation - perhaps worth find-all'ing across the codebase for , callable] so all these are replaced

VeckoTheGecko · 2026-04-17T10:04:34Z

 import yaml

 from virtualship.errors import InstrumentsConfigError, ScheduleError
+from virtualship.instruments.sensors import (


To be clear, are you suggesting from virtualship.instruments import sensors? I think that would be a good idea :)

VeckoTheGecko · 2026-04-17T10:09:06Z


+def build_particle_class_from_sensors(
+    sensors: list[SensorConfig],
+    fixed_variables: list,


Suggested change

fixed_variables: list,

fixed_variables: list[Variable],

VeckoTheGecko · 2026-04-17T10:10:37Z

+def build_particle_class_from_sensors(
+    sensors: list[SensorConfig],
+    fixed_variables: list,
+    particle_class: type,


nitpick. Feel free to ignore this suggestion if you don't find it useful

Suggested change

particle_class: type,

particle_class: type, # generic type annotation needed for v3 particle class behaviour # TODO: Update with Parcels v4

j-atkins added 30 commits March 25, 2026 13:53

remove CTD_BGC instrument type from InstrumentType enum, add SensorTy…

ba585c2

…pe enum

update utils: add sensor def mapping and remove old references to ctd…

1f7e9b8

…_bgc

refactor: update SensorType enum and add source-truth for supported s…

dbaa319

…ensors for instruments

add sensors configuration for various instruments

a2a7c81

new registries and helper functions

67a04d8

update expedition models, now including SensorConfig model and associ…

057d9f9

…ated validations for each instrument

modify adcp instrument class, also abstract expansion to u and v to h…

b82118d

…igher level for scalability

dynamic particle class building takes JIT or Scipy particle

8d96d8f

raise error when instrument has zero sensors enabled

818e8f8

use centralised particle class builder for ADCP now as well

07c8461

batch update instrument subclasses adapted to refactored sensor logic

1bf517e

rename list

961f1fe

adapt argo subclass to sensor refactoring, also separate the sampling…

ee002d7

… kernels from the argo vertical movement kernel to enable easier scalability

consistent particle variable naming

4777217

add back in ctd_bgc for now

c419399

fix import

daabfc4

move sensor information to new sensors.py file

cece7bb

update imports across codebase

b429331

add validator/serialiser for reading from YAML, remove unnecessary pr…

882a419

…operty shorthands

re-add JITParticle to particle class when creating instruments

126ecc2

remove CTD_BGC instrument type from InstrumentType enum, add SensorTy…

9011b2d

…pe enum

update utils: add sensor def mapping and remove old references to ctd…

464b3e9

…_bgc

refactor: update SensorType enum and add source-truth for supported s…

2f7d82d

…ensors for instruments

add sensors configuration for various instruments

1d7c158

new registries and helper functions

f01bf0e

update expedition models, now including SensorConfig model and associ…

d9f9d10

…ated validations for each instrument

modify adcp instrument class, also abstract expansion to u and v to h…

c50c43f

…igher level for scalability

dynamic particle class building takes JIT or Scipy particle

bb91f0c

raise error when instrument has zero sensors enabled

b584d70

use centralised particle class builder for ADCP now as well

6fb6284

j-atkins and others added 24 commits April 8, 2026 14:28

add back in ctd_bgc for now

c9d0623

fix import

8151a60

move sensor information to new sensors.py file

892c75d

update imports across codebase

13ded3f

add validator/serialiser for reading from YAML, remove unnecessary pr…

a30f72b

…operty shorthands

re-add JITParticle to particle class when creating instruments

f0f8a19

Merge branch 'refactor-sensors' of github.com:OceanParcels/virtualshi…

f245288

…p into refactor-sensors

update with new analysis environment

92fe4a1

Merge branch 'new-pixi-env' into refactor-sensors

f4c6205

Merge branch 'main' into refactor-sensors

4351657

fix erroneous sampling during descent and drift

089d2b6

update docstring

96ff22d

Add sensor configuration tests for various instruments and update con…

73474e9

…figurations

new tests for instruments, focused on new sensor logic

c5ec69b

new test_sensors suite

7e9c5e9

new tests for new sensor logic utils

18c1a2d

remove some overlap/duplication of tests

89e184a

deal with circular import issues

bf61220

add ctd_bgc stationkeeping logic back in (accidentally removed earlier)

51f4457

Merge branch 'main' into refactor-sensors

bcbc28f

[pre-commit.ci] auto fixes from pre-commit.com hooks

f9f17f2

for more information, see https://pre-commit.ci

correct comment

548297b

update docstrings, rethink some testing

7a163f7

Merge branch 'refactor-sensors' of github.com:OceanParcels/virtualshi…

6978c95

…p into refactor-sensors

j-atkins marked this pull request as ready for review April 10, 2026 13:34

j-atkins requested review from VeckoTheGecko, ammedd and erikvansebille April 10, 2026 13:34

erikvansebille reviewed Apr 13, 2026

View reviewed changes

VeckoTheGecko reviewed Apr 17, 2026

View reviewed changes

		PRIMARY_PRODUCTION = "PRIMARY_PRODUCTION"


		# per-instrument allowlists of supported sensors (source truth for validation for which sensors each instrument supports)

	sensor_variables = [
	Variable(var_name, dtype=np.float32, initial=np.nan)
	for sc in sensors
	if sc.enabled
	for var_name in sc.meta.particle_vars
	]

	_ARGO_FIXED_VARIABLES = [
	Variable("cycle_phase", dtype=np.int32, initial=0.0),
	Variable("cycle_age", dtype=np.float32, initial=0.0),
	Variable("drift_age", dtype=np.float32, initial=0.0),
	Variable("min_depth", dtype=np.float32),
	Variable("max_depth", dtype=np.float32),
	Variable("drift_depth", dtype=np.float32),
	Variable("vertical_speed", dtype=np.float32),
	Variable("cycle_days", dtype=np.int32),
	Variable("drift_days", dtype=np.int32),
	Variable("grounded", dtype=np.int32, initial=0),
	]

		@@ -50,6 +53,11 @@ def _sample_temperature(particle, fieldset, time):
		particle.temperature = fieldset.T[time, particle.depth, particle.lat, particle.lon]

	particle_class: type,
	particle_class: type, # generic type annotation needed for v3 particle class behaviour # TODO: Update with Parcels v4

Conversation

j-atkins commented Apr 10, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Overview

Major changes

API change

Additional change

Follow-up PRs

Tests

Uh oh!

erikvansebille left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

j-atkins Apr 16, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

VeckoTheGecko left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

j-atkins commented Apr 10, 2026 •

edited

Loading

j-atkins Apr 16, 2026 •

edited

Loading